Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decrypt API Rework #560

Merged
merged 21 commits into from
Aug 20, 2024
Merged

Decrypt API Rework #560

merged 21 commits into from
Aug 20, 2024

Conversation

derekpierre
Copy link
Member

@derekpierre derekpierre commented Jul 30, 2024

Type of PR:

  • Bugfix
  • Feature
  • Documentation
  • Other

Required reviews:

  • 1
  • 2
  • 3

What this does:

  • Rework of decryption API to use ConditionContext instead of splitting up params for authProvider and customParameters.
  • Allow ConditionContext to be populated by developers with authProviders and customParameters before being used for decryption
  • Reduce parameters used in calls for decryption -> retrieve flow. Instead only obtain the ritual from the ritual id when the values are actually needed.

The updated use of the code will look like the following:

  1. Create condition context from message kit:
const conditionContext = conditions.context.ConditionContext.fromMessageKit(messageKit);
  1. Add any required auth provider(s):

    • Add auth provider for :userAddress, if necessary:
    const authProvider = new EIP4361AuthProvider(web3Provider, web3Provider.getSigner());
    conditionContext.addAuthProvider(USER_ADDRESS_PARAM_DEFAULT, authProvider);
    • Add auth provider for :userAddressExternalEIP4361, if necessary:
    const singleSignOnEIP4361AuthProvider = await SingleSignOnEIP4361AuthProvider.fromExistingSiweInfo(messageStr, 
    signature);
    conditionContext.addAuthProvider(USER_ADDRESS_PARAM_EXTERNAL_EIP4361, authProvider);
  2. Add any required custom context variables, if necessary:

const conditionContext = conditions.context.ConditionContext.fromMessageKit(messageKit);
conditionContext.addCustomContextParameterValues({
    ":myCustomValue1": value_1,
    ":myCustomVallue2": value_2,
    ...
});
  1. Call decrypt
const decryptedMessage = await decrypt(web3Provider, domain, messageKit, conditionContext);

Additionally, developers can also better query what parameters are needed for a condition in a message kit. This helps when apps have various permutations of conditions that they use, which include different parameters:

const conditionContext = conditions.context.ConditionContext.fromMessageKit(messageKit);
requestedContextParameters = conditionContext.requestedContextParameters  // set of context variables required for underlying condition
// Devs can check for specific context parameters or loop over the set and populate context variables appropriately.

Issues fixed/closed:

  • Fixes #...

Why it's needed:

Explain how this PR fits in the greater context of the NuCypher Network.
E.g., if this PR address a nucypher/productdev issue, let reviewers know!

Notes for reviewers:

What should reviewers focus on?
Is there a particular commit/function/section of your PR that requires more attention from reviewers?

Copy link

netlify bot commented Jul 30, 2024

Deploy Preview for taco-demo canceled.

Name Link
🔨 Latest commit 1e528aa
🔍 Latest deploy log https://app.netlify.com/sites/taco-demo/deploys/66c34d496107d700081b651e

Copy link

netlify bot commented Jul 30, 2024

Deploy Preview for taco-nft-demo canceled.

Name Link
🔨 Latest commit 1e528aa
🔍 Latest deploy log https://app.netlify.com/sites/taco-nft-demo/deploys/66c34d49e5f0e00008da6d60

@derekpierre
Copy link
Member Author

Replaces #554 which was auto-closed once epic-auth was merged.

packages/taco/src/taco.ts Outdated Show resolved Hide resolved
@derekpierre derekpierre mentioned this pull request Aug 12, 2024
7 tasks
…et the ritual so early; obtaining it later saves us from passing additional parameters.
Allow Context to be created with a condition, and subsequently populated with authProviders and customParameters.
Make :userAddressExternalEIP4361 a reserved context variable because the context will use the provided authProviders to populate its (and :userAddress) values in the overall context used for decryption.
… replaces authProvider and customParmeters since those are now encompassed in the context itself.
…common elements for both ei4361 provider and single sign-on provider.

Move user address context variable for external eip4361 to taco-auth as well.
Update relevant imports.
…d of directly passing auth provider.

Expose USER_ADDRESS_PARAM* constants from taco for now.
@codecov-commenter
Copy link

codecov-commenter commented Aug 15, 2024

Codecov Report

Attention: Patch coverage is 76.44231% with 49 lines in your changes missing coverage. Please review.

Project coverage is 79.10%. Comparing base (e8f9098) to head (1e528aa).
Report is 155 commits behind head on main.

Files Patch % Lines
packages/test-utils/src/utils.ts 0.00% 34 Missing ⚠️
packages/taco/examples/encrypt-decrypt.ts 0.00% 5 Missing ⚠️
packages/taco/src/conditions/context/context.ts 96.77% 2 Missing and 1 partial ⚠️
packages/taco/src/taco.ts 75.00% 1 Missing and 2 partials ⚠️
packages/taco/src/tdec.ts 82.35% 1 Missing and 2 partials ⚠️
...aco-auth/src/providers/eip4361/external-eip4361.ts 85.71% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #560       +/-   ##
===========================================
+ Coverage   23.12%   79.10%   +55.98%     
===========================================
  Files          62       66        +4     
  Lines       10175     6801     -3374     
  Branches      260      300       +40     
===========================================
+ Hits         2353     5380     +3027     
+ Misses       7763     1377     -6386     
+ Partials       59       44       -15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

… then pass the client to calls later in the stack instead of passing around the porterUri(s).
@derekpierre derekpierre changed the title [WIP] API Rework Decrypt API Rework Aug 16, 2024
@derekpierre derekpierre marked this pull request as ready for review August 16, 2024 13:25
Copy link
Contributor

@theref theref left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

requestedContextParameters = conditionContext.requestedContextParameters  // list of context variables required for underlying condition
// Devs can loop over the list and populate context variables appropriately.

Could we get an example of iterating over this?

Otherwise it looks great!!!

@derekpierre
Copy link
Member Author

derekpierre commented Aug 19, 2024

requestedContextParameters = conditionContext.requestedContextParameters  // list of context variables required for underlying condition
// Devs can loop over the list and populate context variables appropriately.

Could we get an example of iterating over this?

Great idea - added an illustrative optional example to the nodejs example - see 1e528aa.

conditionContext.addAuthProvider(USER_ADDRESS_PARAM_DEFAULT, authProvider);

// illustrative optional example of checking what context parameters are required
// unnecessary if you already know what the condition contains
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

beautiful! 🚀

Copy link
Member

@arjunhassard arjunhassard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once we offer lots of authentication methods and broader conditionality, this structural improvement will be invaluable to some use cases!

Comment on lines +118 to +127
conditionContext.requestedContextParameters.has(USER_ADDRESS_PARAM_DEFAULT)
) {
const authProvider = new EIP4361AuthProvider(
provider,
consumerSigner,
siweParams,
);
conditionContext.addAuthProvider(USER_ADDRESS_PARAM_DEFAULT, authProvider);
}
return decrypt(provider, domain, messageKit, conditionContext);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this avoids having to hardcode context parameters – i.e. the app doesn't need to know the conditions in advance in order to match them to the right authentication method? That's pretty amazing!

One thing I'm wondering is – what resource is being queried here? Do we need to create a public resource that maps conditions to context parameters? Or is this a list that developers will create for their own domain?

Copy link
Member Author

@derekpierre derekpierre Aug 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this avoids having to hardcode context parameters – i.e. the app doesn't need to know the conditions in advance in order to match them to the right authentication method? That's pretty amazing!

Indeed. This is the case where an application doesn't just use the same condition every time, but perhaps some permutations of a set of conditions, each with varying context parameters. In this way the dev can use logic to get the set of expected parameters and populate them based on their own logic.

They could have dynamic logic like the following:

if (requestedContextParmeters.has(":tokenId")) {
   // user subjected to condition 1 which uses ":tokenId" custom context parameter
   const value = ...
   conditionContext.addCustomContextParameters({":tokenId": value});
}
if (requestedContextParmeters.has(":otherParam")) {
   // user subjected to condition 2 which uses ":otherParam" custom context parameter
   const otherValue = ...
   conditionContext.addCustomContextParameters({":otherParam": otherValue});
}
if (requestedContextParmeters.has(":userAddress")) {
   // user subjected to condition with taco SIWE
   const authProvider = new EIP4361AuthProvider(...);
   conditionContext.addAuthProvider(":userAddress", auth provider);
}
if (requestedContextParmeters.has(":userAddressEIP4361")) {
   // user subjected to condition with single-sign on SIWE
   const authProvider = SingleSignOnEIP4361AuthProvider.fromExistingSiweInfo(...)
   conditionContext.addAuthProvider(":userAddressEIP4361", authProvider);
}
...

One thing I'm wondering is – what resource is being queried here?

Assuming I correctly understand the question, the ConditionContext looks at the underlying condition in the ThresholdMessageKit (encrypted data), and then returns all the context parameters in the underlying condition (requestedContextParameters property). It is then up to the app to populate the values appropriately. How they do that is up to them - we are not being prescriptive.

Do we need to create a public resource that maps conditions to context parameters? Or is this a list that developers will create for their own domain?

Interesting. To me, it's the latter. Devs can decide how they want to get/generate the values to use for conditions.
IMHO, on the decryption side, we should endeavour for apps not to have to deal with the underlying condition object(string) themselves. They can if they want to, of course, but they shouldn't really have to. It's part of the reason I made the ConditionContext object take a ThresholdMessageKit and not the condition object contained within the ThresholdMessageKit.

With something like a public mapping resource (condition string/type -> context parameters...?), it may be easy for simple conditions eg. BalanceOf, or OwnsNFT, but it's tough cover all cases with a mapping because our conditions are extensible, which is intentional for breadth of functionality. Devs can use whatever custom context parameters they want, and they can also use :userAddressExternalEIP4361 (single sign on SIWE) OR :userAddress (taco SIWE) if they like...it would be tough to cover all cases. Even with the mapping, they'll still need to know/determine what value to use for the context parameters - so they are kind of back to something like the 'if' statements above.

Copy link
Member

@manumonti manumonti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work here! 🙌

@derekpierre derekpierre merged commit 43786ce into nucypher:main Aug 20, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants